London|May-2025|KhilolaRustamova|Module-Structuring-&-Testing-Data|sprint 3 #636
Conversation
HilolaRustam
changed the title
HilolaRustam
added
📅 Sprint 3
illicitonion
added
Review in progress
illicitonion
changed the base branch from
to-be-deleted-coursework/sprint-3
to
main
| @@ -8,7 +8,16 @@ | |||
| // write one test at a time, and make it pass, build your solution up methodically | |||
| // just make one change at a time -- don't rush -- programmers are deep and careful thinkers | |||
| function getCardValue(card) { | |||
|
|
|||
| const rank = card.slice(0, -1); // remove the suit emoji | |||
There was a problem hiding this comment.
This comment explains clearly what's going on - nice comment!
|
|
||
| if (num >= 2 && num <= 9) return num; | ||
|
|
||
| throw new Error("Invalid card rank"); |
There was a problem hiding this comment.
If this exception is thrown, it may not be clear to the caller what the invalid rank was - can you think how to make this more clear?
| if (["K", "Q", "J", "10"].includes(rank)) return 10; | ||
|
|
||
| const num = parseInt(rank); |
There was a problem hiding this comment.
This works, but: either of these lines of code could handle the value 10 - why did you decide to handle it in the K/Q/J case rather than the number case?
There was a problem hiding this comment.
By grouping "10" with "K", "Q", and "J", you make the intention clear:
"These are all cards that are worth 10 points, no matter what."
It also avoids need of extra range check (num === 10) in the number parsing block, which could make the logic more complex.
| @@ -22,3 +22,6 @@ test("should count multiple occurrences of a character", () => { | |||
| // And a character char that does not exist within the case-sensitive str, | |||
| // When the function is called with these inputs, | |||
| // Then it should return 0, indicating that no occurrences of the char were found in the case-sensitive str. | |||
|
|
|||
| console.log(countChar("hello", "l")); // Should print 2 | |||
There was a problem hiding this comment.
These are correct expectations, but they're not tests written in Jest - can you try to write actual tests for these cases?
|
|
||
| }); | ||
| // output | ||
| console.log(getOrdinalNumber(1)); // "1st" |
There was a problem hiding this comment.
Again, these are fine examples, but they're not jest tests - can you turn them into jest tests?
| @@ -13,20 +13,36 @@ test("should repeat the string count times", () => { | |||
| const str = "hello"; | |||
| const count = 3; | |||
| const repeatedStr = repeat(str, count); | |||
| expect(repeatedStr).toEqual("hellohellohello"); | |||
| expect(repeat(str, count)).toEqual("hellohellohello"); | |||
There was a problem hiding this comment.
Why did you change this line? What was broken about what it did before?
There was a problem hiding this comment.
expect(repeat(str, count)).toEqual("hellohellohello");
Calls the repeat() function directly inside the test.
It’s like saying: “Call the function now with str and count, and I expect it to return "hellohellohello".”
before it was saying "I expect the value stored in repeatedStr to equal "hellohellohello"
It might not be broken but i find it unnecessary step
illicitonion
added
Reviewed
HilolaRustam
added
Needs Review
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.